Skip to content

🏯 fix: Guard Worker Credentials From Local Accounts - #112

Merged
danny-avila merged 1 commit into
mainfrom
harden/local-account-credentials
Sep 4, 2026
Merged

🏯 fix: Guard Worker Credentials From Local Accounts#112
danny-avila merged 1 commit into
mainfrom
harden/local-account-credentials

Conversation

@danny-avila

@danny-avila danny-avila commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Follow-up to #107, now merged. That PR fixed a reproducible defect — chmod silently no-ops on WSL2 DrvFs, so the worker's Ed25519 private key was written world-readable while pairing reported success. This one is different in kind, and needs its own decision.

The question this PR is really asking

Is another local account on the same host in scope for BYOM?

Everything here defends that threat. BYOM's stated model is a worker on the user's own machine or VM, with the trusted worker outside the sandbox and the sandboxed command as the adversary. Under that model none of this is needed. Under a shared or managed host, all of it is.

It is separate because these checks buy that defence by trading deployment flexibility, and that trade should be made deliberately rather than accumulated under review pressure. Two of them can refuse setups that work today:

  • Container ownership refuses an identity in a directory owned by a provisioning or service account — plausible in containerized deployments.
  • Default-workspace ownership refuses a pre-provisioned workspace directory owned by another account.

If the answer to the question above is "no", most of this should be dropped rather than merged. I would rather ask than assume.

What it adds

Ownership. Mode bits do not establish trust: a 0600 file owned by another account is unreadable by others yet fully rewritable by its owner, who then controls the credential the worker loads — or, for a quarantine marker, can delete it and let mutations resume. The containing directory is judged the same way, since an owner lacking write bits today can grant them tomorrow. Root counts as the trust root. --default-workspace is application-owned by contract, so a pre-existing one under another account is refused too.

Containers. A 0600 file in a directory others can write can be unlinked and replaced. Publishing goes through rename, which replaces the named entry, so the write path judges the entry's directory; reading follows the link, so both ends are judged. The sticky bit counts as protection, keeping /tmp-style parents usable.

Pairing preflight. pair redeemed the one-time code before the destination was known usable, so an unusable path cost the code and left an orphaned remote pairing — reproduced with --identity <existing dir>, which surfaced a raw EISDIR after the code was spent. The destination is now validated and then claimed, so another account cannot take the name while the request is in flight. The claim records its inode and is released only if the file is still that inode and still empty, so a concurrent pairing that published a real identity over the name is never destroyed by another invocation's unwind.

Verification

before after
pair --identity <existing dir> raw EISDIR, code spent rejected in preflight; the same code then pairs successfully
pair into a 0500 directory EACCES at save, code spent rejected in preflight
concurrent pair, first one unwinds deletes the other's credential the published identity survives
pair with an invalid code no stray placeholder left behind
credential in a world-writable directory trusted refused (sticky parents still accepted)
symlink whose entry is in a shared directory trusted refused
pair to ext4 / default ~/.config, and re-pairing 0600 0600 (unchanged)

Measured against real paths before committing to the rule: ~ is 0750 and ~/.config 0751 here, both accepted; /tmp at 1777 is accepted via the sticky bit. A naive & 0o077 ancestor check would have rejected the default identity path, which is why the rule is write-bits-plus-sticky rather than mode-wide.

41/41 focused tests (storage, cli, workspace-cli — the only dependents of the changed functions). tsc --noEmit clean.

Known gaps, left explicit rather than half-done

  • Only the immediate container is checked. A writable ancestor could still rename a private directory out from under the worker.
  • Bind-mounted destinations still fail at rename with EBUSY. Every detection I found either false-positives on btrfs subvolumes (st_dev) or races (/proc/mounts), and a test-rename would clobber the file it protects.
  • macOS extended ACLs and Windows ACLs are both outside what a mode check can see. Node exposes no ACL API, so this needs a subprocess validated on a host that has those platforms — neither is available here, and an unverified subprocess in the credential path is worse than a stated limit.

The sticky-plus-foreign-owner reject path cannot be exercised without root, so the test there covers the regression instead: an owned file in a sticky directory stays replaceable.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 4, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-04T17:54:15.633976Z a2b6c3c Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 21c893a27a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +383 to +386
if (!isRecord(error) || error.code !== 'EEXIST') throw error;
/* Already present: judge what is there instead of the placeholder. */
await assertOwnedByWorker(path);
await assertOwnerOnlyPath(path);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Probe parent writability for an existing identity

When re-pairing over an owner-only identity whose parent is private but not writable by the worker, such as an existing file in a 0500 directory, this EEXIST branch passes because it checks only ownership, mode, and shared writability. pair() then redeems the one-time code, after which saveBridgeIdentity() fails to create its sibling temporary file with EACCES, leaving the remote pairing orphaned—the exact failure the preflight is intended to prevent. Verify that the temporary-file-and-rename workflow is usable before allowing the pairing request.

Useful? React with 👍 / 👎.

Comment on lines +383 to +386
if (!isRecord(error) || error.code !== 'EEXIST') throw error;
/* Already present: judge what is there instead of the placeholder. */
await assertOwnedByWorker(path);
await assertOwnerOnlyPath(path);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Treat the placeholder as an active reservation

When two pair invocations target the same previously absent identity path, the first creates the placeholder, but the second handles its EEXIST as though it were a pre-existing identity and proceeds with pairing. If the second saves its credential and the first subsequently fails, the first reservation's release() removes the second command's completed identity, consuming its code and leaving no local credential. Distinguish an active placeholder from an existing identity or make release conditional on the path still naming the reserved file.

Useful? React with 👍 / 👎.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

Both fixed in 563221e.

Treat the placeholder as an active reservation — this was the serious one, and it was mine: release() removed the destination whenever this call had created it, without checking the file still was its placeholder. Two concurrent pair runs against the same absent path made that destructive exactly as described — the second treats the placeholder as pre-existing, publishes a real identity over the name, and the first's unwind deletes a credential whose one-time code is already spent. Worse outcome than the burned code the preflight exists to prevent.

Now the placeholder's inode is recorded at creation, and release() removes the file only if it is still that inode and still empty. Regression test covers the interleaving directly: reserve, let another pairing publish over it, release, assert the published identity survives.

Probe parent writability for an existing identity — right, and it is the same shape as the EISDIR case this preflight already handles. The pre-existing-destination branch judged only the file, but publishing writes <path>.<random>.tmp beside it and renames over, so a 0500 directory holding a readable owner-only identity passed everything and still failed the save with EACCES. It now exercises the sibling write instead of inferring it, with a test that makes the directory 0500 and expects the rejection.

Verified end to end: first pairing, re-pairing over an existing identity (the EEXIST branch, now including the publish probe), and an invalid code leaving no placeholder. 41/41 focused tests.

@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7d54ec234a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +214 to +215
if ((mode & 0o1000) !== 0 && (metadata.uid === uid || metadata.uid === 0)) {
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject foreign-owned symlinks in sticky directories

When an identity path is a symlink owned by another account inside a sticky shared directory such as /tmp (on macOS or Linux with protected-symlink enforcement disabled), this return treats the entry as protected merely because the directory is sticky. Sticky semantics still allow the symlink's owner to remove and recreate that entry, while readGuardedFile checks only the followed target's owner and mode; consequently that account can redirect loadBridgeIdentity to another worker-owned identity or repeatedly break startup. Check the entry's ownership when relying on the sticky bit, especially for symlinks.

Useful? React with 👍 / 👎.

Comment on lines +641 to +644
await chmod(locked, 0o500);
await assert.rejects(
assertIdentityPathIsPrivate(identityPath),
/could not be published there/,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Skip the permission-denial test for privileged runners

When this suite runs as root or with CAP_DAC_OVERRIDE, as is common in containerized test jobs, changing the directory to 0500 does not prevent creation of the sibling probe. assertIdentityPathIsPrivate therefore resolves successfully and this assertion fails with Missing expected rejection; running node --test dist/storage.test.js as uid 0 reproduces it. Run this case as an unprivileged user or skip it when the process can bypass discretionary access controls.

Useful? React with 👍 / 👎.

/* Already present: judge what is there instead of the placeholder, and
* prove the publish itself is possible - an unwritable parent would
* otherwise surface as EACCES only after the code was spent. */
await assertOwnedByWorker(path);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Allow replacing foreign entries in caller-owned sticky directories

When the identity lives in a shared sticky directory owned by the worker account and another account has pre-created the destination, POSIX sticky-directory rules allow the directory owner to replace that entry safely. assertIdentityDestinationIsReplaceable explicitly recognizes this case, but this unconditional target-ownership check rejects it immediately afterward; as a result, another local account can block pairing merely by reserving the predictable filename even though the eventual rename would succeed and the replacement would be protected. Skip this ownership requirement when the caller owns the sticky parent while retaining it for identity loads.

Useful? React with 👍 / 👎.

Base automatically changed from fix/private-mode-verification to main September 4, 2026 17:49
Everything here defends a threat the merged owner-only fix does not: another
account on the same host. It is separate deliberately, because BYOM's stated
model is a worker on the user's own machine or VM with the sandboxed command as
the adversary, and these checks buy that defence by trading deployment
flexibility for it.

Ownership. Mode bits do not establish trust: a 0600 file owned by another
account is unreadable by others yet fully rewritable by its owner, who then
controls the credential the worker loads - or, for a quarantine marker, can
delete it and let mutations resume. The containing directory is judged the same
way, since an owner lacking write bits today can grant them tomorrow. Root
counts as the trust root. `--default-workspace` is application-owned by
contract, so a pre-existing one under another account is refused too.

Containers. A 0600 file in a directory others can write can be unlinked and
replaced. Publishing goes through `rename`, which replaces the named entry, so
the write path judges the entry's directory; reading follows the link, so both
ends are judged. The sticky bit counts as protection, keeping /tmp-style
parents usable. Only the immediate container is inspected.

Pairing preflight. `pair` redeemed the one-time code before the destination was
known usable, so an unusable path cost the code and left an orphaned remote
pairing. The destination is now validated - rejecting a directory, a
foreign-owned file under a sticky bit that `rename` could not replace, and a
parent that denies the sibling temporary file the publish needs - and then
claimed, so another account cannot take the name while the pairing request is
in flight. The claim records its inode and is released only if the file is
still that inode and still empty, so a concurrent pairing that published a real
identity over the name is never destroyed by another invocation's unwind.

Known gaps, left explicit rather than half-done: only the immediate container
is checked, so a writable ancestor could still rename a private directory out
from under the worker; a bind-mounted destination still fails at `rename` with
EBUSY because every way to detect it either false-positives on btrfs
subvolumes or races; and macOS extended ACLs and Windows ACLs are both outside
what a mode check can see, which needs real ACL inspection on a host that can
validate it.
@danny-avila
danny-avila force-pushed the harden/local-account-credentials branch from 7d54ec2 to a2b6c3c Compare September 4, 2026 17:50
@danny-avila

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: a2b6c3c448

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@danny-avila danny-avila changed the title feat(code): harden worker credentials against other local accounts 🏯 fix: Guard Worker Credentials From Local Accounts Sep 4, 2026
@danny-avila
danny-avila merged commit 81610a7 into main Sep 4, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants